Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

webextensions: add a portal for managing WebExtensions native messaging servers #1537

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

xhorak
Copy link
Contributor

@xhorak xhorak commented Dec 17, 2024

This MR continues the work from the abandoned MR #705.

The following updates and improvements have been made:

  • rebase
  • annotation for qt
  • fixed test

@jhenstridge thank you for your work on this! Let me know if you'd like to collaborate further.

This is intended to provide a way for a confined web browser to start native code helpers for their extensions. At present it can start the servers installed on the host system. But in future this could be extended to cover sandboxed native messaging servers too.

@Rob--W
Copy link

Rob--W commented Dec 17, 2024

Has the other PR really been abandoned, or is the author merely focused on other tasks or even away from work? The latter would not be surprising considering that we are approaching the end of the year.

This patch squashes all changes from #705 into one, along with new changes. I suppose that this was the easiest way to rebase, but it causes the lost of context because some individual commits offered context that are missing from the squashed commit.

Out of all, I think that it would make most sense to include the context of 59d7b4b addresses #769 . E.g. by including the issue reference in the commit message ("Fixes xxx") and a code comment pointing to the discussion.
I have not reviewed the updated patch yet, but note that in the previous PR there was still an active discussion on this topic at #705 (comment)

@swick
Copy link
Contributor

swick commented Dec 17, 2024

I'd really like if the test was moved over to python.

@xhorak
Copy link
Contributor Author

xhorak commented Dec 17, 2024

I've been trying to contact authors since late October but did not received any reply. I would happily keep the original PR but I want to move this forward. Especially now when the Firefox part landed.

The squash was suggested, so I did it. I'll check the 59d7b4b
I plan to do some testing because I'm getting various app_ids as I've mentioned in #705 (comment)

@xhorak
Copy link
Contributor Author

xhorak commented Dec 17, 2024

In latest update I've fixed the xdp_app_info_get_gappinfo call and stopped to use g_desktop_app_info_new.

@grulja
Copy link
Contributor

grulja commented Dec 18, 2024

I plan to do some testing because I'm getting various app_ids as I've mentioned in #705 (comment)

That's the usual issue we have with host apps. The application id comes from cgroups, which is set depending on the way you start the app, e.g. you start it from a terminal app it might get app id of the terminal app. Or you start it with Alt + F2 in GNOME and the app id will be just firefox. Flatpak apps are reliable in this case.

See #1512 what we did for the camera portal, where this was causing issues.

@swick
Copy link
Contributor

swick commented Dec 18, 2024

Regarding the appid mismatch that unfortunately still happens: I'd much rather see us adding #1521 than special casing every portal that firefox actually depends on.

@xhorak
Copy link
Contributor Author

xhorak commented Dec 19, 2024

I'd really like if the test was moved over to python.

Looking at the python vs C tests, there seems to be missing permission store service in Python. I'm not sufficiently familiar with the code to implement it for python tests or is it done somewhere already?

@swick
Copy link
Contributor

swick commented Dec 19, 2024

The permission store is supposed to get started (https://github.com/flatpak/xdg-desktop-portal/blob/1c902cc77e53b422d49988bab58b6cca0ed9b112/tests/conftest.py#L458C5-L458C25) when the test case uses either the xdg_permission_store or the more general portals fixtures.

xhorak added 2 commits January 8, 2025 10:16
…ng servers.

Rebase, fix and continue work on webextensions: add a portal for managing WebExtensions
native messaging servers:
flatpak#705

This commit builds on the work done in the original MR authored by @jhenstridge
but resolves pending items and brings it closer to completion.

This is intended to provide a way for a confined web browser to start
native code helpers for their extensions. At present it can start the
servers installed on the host system. But in future this could be
extended to cover sandboxed native messaging servers too.

Fixes: flatpak#769
@xhorak
Copy link
Contributor Author

xhorak commented Jan 8, 2025

The proposal (by @swick) suggests replacing xdg-desktop-portal with a separate service, org.freedesktop.native_messaging, for native messaging API support. This would make the feature exclusive to browsers using the --talk-name=org.freedesktop.native_messaging permission. On the other hand it removes the user access dialog provided by xdg-desktop-portal.

The service API remains unchanged, but as noted by @swick, this approach does not address potential sandbox escapes but it will be exclusive to those who want to use the org.freedesktop.native_messaging.

@Rob--W please share your thoughts from the Firefox point of view and all the work that has been done on the Firefox side.

@Rob--W
Copy link

Rob--W commented Jan 8, 2025

Could you elaborate on the changes that you're considering? What are the current values and the proposed values?

It sounds like the proposal may effectively introduce a backwards-incompatible change, requiring changes to https://searchfox.org/mozilla-central/rev/7d1b5c88343879056168aa710a9ee743392604c0/toolkit/components/extensions/NativeMessagingPortal.cpp#110-112, and maybe other places too.

The feature in Firefox is currently disabled by default to offer room for such breaking changes if really needed. I would imagine Canonical to either apply distro-specific patches to Firefox, or update the portal to transition to the new state. But I cannot speak on behalf of them, so once you've clarified the exact changes that you're considering, I'll ping them.

@xhorak
Copy link
Contributor Author

xhorak commented Jan 8, 2025

Could you elaborate on the changes that you're considering? What are the current values and the proposed values?

@swick I'm not sure how the separate service will support creating and maintaining sessions currently implemented by xdp. Could you help?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Needs Triage
Development

Successfully merging this pull request may close these issues.

5 participants